fix(prune): detect orphan skills and preserve retained bundles - #3057

python-architect

• [recommended] Standalone SKILL.md ancestor containing a declared child is orphaned and deleted, destroying the child. at src/apm_cli/commands/_helpers.py:355
  _find_orphaned_packages (line 355) protects children of expected roots via path.startswith(f"{root}/") but does not protect ancestors that CONTAIN expected children. _expand_with_ancestors (line 244) skips standalone ancestors (ancestor in installed_set and ancestor not in materialized_set). In the scenario where owner/repo has SKILL.md (newly recognized) and owner/repo/skills/child is declared, owner/repo is standalone, excluded from ancestor expansion, classified as orphan, and deleted by safe_rmtree -- destroying the declared child. Before this PR, SKILL.md was not recognized by _scan_installed_packages, so the root was invisible to prune and the child was safe. The test test_skill_only_orphan_root_is_not_hidden_by_declared_subdirectory (line 391) encodes this broken behavior. Brief known_gaps[0] calls this 'Required change is ancestor retention, no waiver as inherited behavior.' The approved plan addresses it with 'indexed retained ancestors'. A reverse-containment guard in _find_orphaned_packages (not any(root.startswith(f"{path}/") for root in expected)) would close the gap.

Design patterns

• Used in this PR: Pure Function extraction -- _find_orphaned_packages centralizes orphan selection as a side-effect-free function shared by _check_orphaned_packages (warning) and prune (deletion), eliminating the prior split authority.
• Used in this PR: Static Boundary Guard -- check_orphan_selection + mutation test enforces both call sites route through the canonical owner, registered in install-deployment.json.
• Pragmatic suggestion: none -- the current shape is the simplest correct design at this scope; the ancestor fix is a predicate addition, not a pattern change.
  Suggested: Add reverse-containment check: and not any(root.startswith(f"{path}/") for root in expected). This protects ancestors that contain declared children without weakening sibling orphan detection. Update test_skill_only_orphan_root_is_not_hidden_by_declared_subdirectory to assert ancestor retention instead of deletion.
• [nit] Architecture mutation test covers only the prune.py call site of the dual-site orphan-selection guard. at tests/integration/test_architecture_owner_rule_mutations.py:78
  check_orphan_selection verifies both _check_orphaned_packages and prune route through _find_orphaned_packages. The mutation test (test_architecture_owner_rule_mutations.py) only mutates prune.py. Adding a matching MutationCase for _check_orphaned_packages would close the guard verification for both monitored paths.

test-coverage-expert

• [blocking] test_skill_only_orphan_root_is_not_hidden_by_declared_subdirectory asserts recursive deletion of an ancestor whose subtree contains a still-declared child dependency at tests/unit/test_prune_command.py:391
  Traced _find_orphaned_packages for the test scenario: installed=['owner/repo','owner/repo/skills/child'], expected={'owner/repo/skills/child'}, standalone=['owner/repo']. _expand_with_ancestors SKIPS 'owner/repo' as a standalone ancestor (not declared), producing expected_with_ancestors={'owner/repo/skills','owner/repo/skills/child'}. The child-protection filter checks path.startswith(f'{root}/') for root in expected, but 'owner/repo'.startswith('owner/repo/skills/child/') is False, so 'owner/repo' passes through as an orphan. safe_rmtree (wrapping shutil.rmtree) on apm_modules/owner/repo recursively deletes the entire tree including owner/repo/skills/child -- a DECLARED dependency. The test codifies this with assert root.exists() == dry_run and never checks child.exists() in non-dry-run mode. The code is missing the symmetric reverse check: not any(root.startswith(f'{path}/') for root in expected), which would protect ancestors containing declared descendants. The brief explicitly requires: 'current unsafe expected child deletion must be reversed' and 'Keep such ancestor roots intact; no selective descendant carve-out.' No test at any tier asserts the correct behavior (ancestor retention when containing a declared child). grep'd tests/ for ancestor.*retain, retain.*ancestor, protect.*ancestor: no match asserting retention.
  Suggested: Fix _find_orphaned_packages in src/apm_cli/commands/_helpers.py to add the reverse ancestor check: 'and not any(root.startswith(f"{path}/") for root in expected)'. Then reverse the test assertion to: assert root.exists() (root is retained in all modes), assert (child / "SKILL.md").read_text() == "# Child skill\n", assert "No orphaned packages" in result.output. Add a corresponding lifecycle-tier test in tests/integration/test_prune_skill_lifecycle.py (e.g. test_prune_retains_standalone_ancestor_containing_declared_child) using real git install + declaration removal + prune to prove the ancestor survives with its declared child intact.
  Evidence (missing, integration-with-fixtures): tests/unit/test_prune_command.py -- Prune never removes a standalone ancestor directory when it contains a still-declared child dependency
• [recommended] Windows CI collection roots do not include tests/integration/test_prune_skill_lifecycle.py despite windows_compat marker on the new lifecycle file at .github/workflows/ci.yml:156
  The windows-compat-gate CI job collects 'tests/unit tests/integration/test_lifecycle_workspace_lock.py' at line 154-156 of ci.yml. The new test_prune_skill_lifecycle.py carries pytestmark=[pytest.mark.windows_compat] at module scope, but the marker only filters WITHIN collected paths -- it does not expand the collection. The lifecycle tests will not execute on Windows CI. The original issue [BUG] apm prune does not delete packages from apm_modules #3015 was reported on Windows 11/Python 3.13. The approved plan acknowledges this: 'Bounded Windows repair changes collection roots to tests/unit tests/integration under existing windows_compat marker, same job/permissions/timeout.' Until that repair lands, the SKILL.md recognition and prune lifecycle contracts have no Windows execution evidence. Existing test_prune_failure_lifecycle.py and test_prune_hook_reconciliation_e2e.py in tests/integration are also outside the collection roots and likewise do not run on Windows.
  Suggested: Expand collection roots in windows-compat-gate to 'tests/unit tests/integration' (the approved plan). Verify the windows_compat marker gates appropriately so only marked tests execute, keeping job runtime bounded. This is planned work; do not block on it, but track it as a required follow-up before the next release.
  Evidence (unknown, integration-with-fixtures): tests/integration/test_prune_skill_lifecycle.py -- Prune skill lifecycle contracts execute on Windows where the original issue 3015 was reported

devx-ux-expert

• [blocking] _find_orphaned_packages marks ancestor roots as orphans even when they contain a still-declared child, causing safe_rmtree to destroy declared dependency content at src/apm_cli/commands/_helpers.py:355
  Package manager cleanup invariant: prune removes only content the user no longer needs. npm prune, pip, cargo clean -- none delete content still declared in the manifest. _find_orphaned_packages (line 355) guards descendants of expected roots (path.startswith(root/)) but never checks the reverse: whether any expected path is a descendant of the candidate orphan. When owner/repo has SKILL.md (standalone) and owner/repo/skills/child is declared, owner/repo is correctly excluded from ancestor expansion (installed but not declared), but then nothing prevents it from being marked orphaned. safe_rmtree on apm_modules/owner/repo recursively deletes the entire tree including the declared child. The user's next apm compile or apm run fails with missing dependency content, and recovery requires a full apm install. The test test_skill_only_orphan_root_is_not_hidden_by_declared_subdirectory (line 408: assert root.exists() == dry_run) encodes this destructive behavior as correct, contradicting the brief's required ancestor retention policy.
  Suggested: Add a reverse-descendant guard: and not any(root.startswith(f"{path}/") for root in expected). Fix the test at test_prune_command.py:408 to assert ancestor retention when it contains a declared child.
  Evidence (code-read only; no execution, unit): tests/unit/test_prune_command.py -- Test encodes destructive behavior: prune destroys an ancestor root whose subtree contains a declared dependency, violating the cleanup-only contract.
• [nit] prune.md note implies SKILL.md detection is protective but the detection feeds the deletion path for ancestor roots at docs/src/content/docs/reference/cli/prune.md:128
  The new note at line 128 ('Manifestless installs containing SKILL.md are detected even after apm install removes their lockfile entry') reads as a safety assurance: we find these packages so they participate in cleanup decisions. But detection of a SKILL.md root is what makes it standalone, which is what makes it eligible for orphan marking, which triggers deletion even when it contains declared children (see blocking finding). Until the ancestor guard is fixed, 'detected' effectively means 'eligible for deletion' for ancestor roots. The note is not technically false but primes the user to expect safe handling that does not exist.

supply-chain-security-expert

• [blocking] Pruning a SKILL.md-recognized standalone orphan root recursively destroys its declared child because _find_orphaned_packages lacks a reverse-ancestor containment guard at src/apm_cli/commands/_helpers.py:355
  _find_orphaned_packages (line 340) protects descendants of expected roots via path.startswith(f"{root}/"), but never checks whether a candidate orphan is an ANCESTOR of expected content. When a SKILL.md-bearing root (e.g. owner/repo) is classified standalone by the new scanner, _expand_with_ancestors correctly excludes it from ancestor expansion (it is standalone, not an intermediary). But _find_orphaned_packages then classifies the root as orphaned because it is not in expected_with_ancestors and is not a descendant of any expected path. prune.py line 254 calls safe_rmtree on the entire subtree, destroying the declared child (e.g. owner/repo/skills/child) along with the root. safe_rmtree's ensure_path_within guard only enforces containment within apm_modules -- it has no visibility into whether the subtree contains declared dependencies. This is a newly introduced reachable path: before this PR, directories with only SKILL.md were invisible to _scan_installed_packages, so this ancestor-deletion scenario was unreachable. The test test_skill_only_orphan_root_is_not_hidden_by_declared_subdirectory codifies the destructive behavior with assert root.exists() == dry_run. The brief's known_gaps[0] confirms: 'Required change is ancestor retention, no waiver as inherited behavior.'
  Suggested: Add a reverse-ancestor guard to _find_orphaned_packages: before classifying a path as orphaned, check not any(root.startswith(f"{path}/") for root in expected) to protect ancestors of declared content. Fix the test assertion to expect ancestor retention (root.exists() == True regardless of dry_run). This keeps the standalone-orphan detection for genuine orphans while preventing recursive destruction of declared descendants.
  Evidence (unknown, unit): tests/unit/test_prune_command.py -- Test codifies destructive ancestor deletion of declared child content; if it passes, it proves the declared-content-preservation contract is violated.
• [nit] Prior panel-finding audit trail about forgeable marker heuristic removed from _standalone_installed_packages docstring at src/apm_cli/commands/_helpers.py:265
  The old docstring explicitly acknowledged a prior panel finding: 'Combining both signals closes the suppression-via-absence gap (panel finding: forgeable apm.yml heuristic)'. This was replaced with 'Package markers preserve standalone orphan detection even when a declaration points at a subdirectory of a removed package root.' With SKILL.md now extending the forgeable marker surface (an attacker who can write into apm_modules/ can drop either apm.yml or SKILL.md to influence standalone classification), preserving the forgeable-marker acknowledgment keeps the known weakness visible to future security reviewers without changing any runtime behavior.

performance-expert

• [recommended] O(IE) linear prefix scan in _find_orphaned_packages should be O(ID) via set-indexed segment walk at src/apm_cli/commands/_helpers.py:355
  Line 355 iterates every root in expected for every installed path: any(path.startswith(f"{root}/") for root in expected). This is O(IEL) where I=installed, E=expected, L=avg string length. For 100 installed x 80 expected x 30-char avg that is ~240k char comparisons per invocation. The function runs on both the warning path (compile/install via _check_orphaned_packages) and the deletion path (prune), so the cost is paid on every CLI command that checks for orphans. The expected set is already typed as set[str]; splitting each installed path into segments and checking each segment-delimited prefix against the set replaces the inner O(E) scan with O(D) set lookups (D = path depth, bounded at 2-6 by install conventions). This is the canonical 'x in list inside a loop -> recommend set/dict index' pattern from the algorithmic-patterns lens. Absolute cost today is sub-ms for typical projects, but the pattern compounds linearly with dep count and is trivially fixable.
  Suggested: Replace the inner any(path.startswith(...)) with a segment-bounded prefix walk against a frozenset:

  expected_frozen = frozenset(expected)
  def _is_child_of_expected(p: str) -> bool:
  parts = p.split("/")
  return any("/".join(parts[:i]) in expected_frozen for i in range(1, len(parts)))

  return sorted(
  path for path in installed
  if path not in expected_with_ancestors
  and not _is_child_of_expected(path)
  )

Build expected_frozen once outside the comprehension. Complexity drops from O(IE) to O(ID) where D<=6. Scaling guard: parameterize a benchmark at N and 10*N expected paths, assert wall-time ratio stays below 2x.

• [recommended] Redundant _expand_with_ancestors call: prune.py:134 and _find_orphaned_packages:350 compute the identical expansion at src/apm_cli/commands/prune.py:148
  prune.py line 134 calls _expand_with_ancestors(expected_installed, standalone_installed) and stores the result in expected_with_ancestors for the missing_orphaned_keys comprehension at line 155. Then line 148 calls _find_orphaned_packages which internally calls _expand_with_ancestors(expected, standalone) with the same two arguments, computing the identical set a second time. _expand_with_ancestors does list+set construction, path validation via validate_path_segments (regex), string splitting, and up to 3 ancestor join operations per expected path -- roughly O(E*3) string ops + O(S) set construction per call. For 80 expected + 50 standalone this is ~500 operations duplicated. The fix is to accept an optional pre-computed expanded set in _find_orphaned_packages, or restructure prune.py to pass its already-computed result. The warning path (_check_orphaned_packages) does not have this duplication since it only calls _find_orphaned_packages once, so the fix is prune-specific. Redundant computation pattern from the algorithmic lens.
  Suggested: Add an optional expected_with_ancestors parameter to _find_orphaned_packages:

  def _find_orphaned_packages(
  installed, expected, standalone,
  *, _precomputed_ancestors: set[str] | None = None
  ) -> list[str]:
  expanded = _precomputed_ancestors or _expand_with_ancestors(expected, standalone)
  ...

In prune.py pass the already-computed set:

orphaned_packages = _find_orphaned_packages(
    installed_packages, expected_installed, standalone_installed,
    _precomputed_ancestors=expected_with_ancestors,
)

_check_orphaned_packages continues without the kwarg and pays the single call.

doc-writer

• [recommended] State the approved whole-root deletion tradeoff. at docs/src/content/docs/reference/cli/prune.md:128
  The new note explains SKILL.md discovery but omits its user-visible consequence. src/apm_cli/commands/deps/_utils.py:29-34 recognizes roots by package markers, and src/apm_cli/commands/prune.py:250-254 removes the selected directory wholesale. Manually copied packages and personal files inside an eligible root can therefore be removed without a surviving lock entry. The deployment-ownership protections described elsewhere on this page do not protect those source-root contents.
  Suggested: Add a terse sentence near the orphan definition: "Recognized roots under apm_modules/ are managed installation content. Pruning an eligible root removes its contents, including manually added files; preview with apm prune --dry-run." Keep this definition canonical. Consolidate the duplicate prune rows in packages/apm-guide/.apm/skills/apm-usage/commands.md:25-27 into one short summary and reference, offsetting the added prose. Do not introduce receipt or .apm-pin ownership requirements.
• [recommended] Replace the ancestor-deletion note alongside the retention fix. at docs/src/content/docs/reference/cli/prune.md:131
  The revised note still endorses removing a recognized ancestor of a needed subdirectory. That contradicts the approved requirement to retain the entire ancestor root. Current src/apm_cli/commands/_helpers.py:238-245 excludes recognized ancestors from protection, while :350-355 protects descendants of expected roots, not ancestors containing them. This is not resolved by documenting bundle-descendant preservation alone.
  Suggested: When the driver fixes selection, replace this implementation-oriented bullet with: "Declared direct and dev dependencies, retained transitives, and their bundled skills are preserved. An ancestor root containing a needed dependency is retained intact; an unrelated sibling root remains eligible for pruning." Do not claim this behavior before the code fix. Update the existing CHANGELOG.md:44 entry to summarize both manifestless cleanup and needed-child/ancestor preservation rather than adding another entry.
• [recommended] Distinguish Windows marker selection from Windows execution evidence. at docs/src/content/docs/contributing/integration-testing.md:386
  The contributor guidance mentions bounded collection roots without naming them. .github/workflows/ci.yml:152-157 currently filters windows_compat only within tests/unit and tests/integration/test_lifecycle_workspace_lock.py. Marking a different integration file does not include it in that job. The supplied packet identifies precisely this lifecycle-selection gap; the marker itself establishes neither collection nor a successful Windows run.
  Suggested: Replace the paragraph alongside the approved CI repair with the exact selection: windows_compat within tests/unit and tests/integration. Explain briefly that collection confirms inclusion, while a successful Windows job provides execution evidence. Retain the non-empty-subset and timeout guidance, and do not describe the widened roots as current until ci.yml changes.

cli-logging-expert

No findings.

python-architect

No findings.

test-coverage-expert

• [recommended] Actual Windows execution of the mixed windows_compat selection remains unverified at final head
  CI run 36108480058 shows action_required with zero jobs executed. The 566 windows_compat-marked unit and integration tests ran on macOS at older commit 5feddb6, not at final head 2e8c618 and never on a Windows runner. macOS execution does not constitute Windows portability evidence. No code change is currently indicated; the gap is resolved by a human approving the pending workflow run so the Windows runner executes the selection against the final head.
  Suggested: A maintainer must approve the pending workflow run so the Windows selection executes at the final head. Until that run completes green, Windows execution remains unverified.
  Evidence (unknown, e2e): tests/integration/test_prune_skill_lifecycle.py -- Actual Windows execution of the original install/remove/install/prune contract remains unverified.

supply-chain-security-expert

No findings.

performance-expert

No findings.

doc-writer

No findings.